checkpointer: implement state machine. #755

diegs · 2017-10-28T00:26:39Z

This implements an explicit state machine to capture the various states
a checkpoint can be in. It recovers those states from disk upon startup
(if applicable) and then reconciles them with the states that are
fetched from the various apiservers.

It also adds 2 new states to attempt to workaround issues with
kubernetes 1.8 in which daemonset pods are deleted before being
recreated. For single-master clusters this can lead to a total outage
during apiserver upgrades since the checkpointer will aggressively
retire the checkpoint for the deleted apiserver pod before it has
a chance to schedule the new one.

diegs · 2017-10-31T00:26:10Z

coreosbot run e2e checkpointer

diegs · 2017-10-31T04:37:54Z

cc @thorfour

diegs · 2017-10-31T04:38:13Z

coreosbot run e2e checkpointer

mxinden

@diegs This looks promising. Thanks for the update!

Would you like us to test this on coreos/tectonic-installer#1938 before merging here?

diegs · 2017-10-31T16:20:52Z

@mxinden i am going to test this against a tectonic cluster this morning.

aaronlevy

Just a couple minor comments, and somewhat quick pass (probably good to have another pair of eyes take a look at)

aaronlevy · 2017-10-31T18:16:58Z

pkg/checkpoint/process.go

+				state: stateNone{},
+			}
+		}
+		// Always overwrite with the localParentPod since it is a better source of truth.


Better source of truth than what (and for what)? Do we not just use name/namespace (I wouldn't expect that to change).

Well, it's using this pod to write to disk so we want to make sure we have the most updated copy of it right?

aaronlevy · 2017-10-31T18:29:39Z

pkg/checkpoint/process.go

-		if !ok || isPodCheckpointer(inactiveCheckpoints[id], checkpointerPod) {
-			glog.V(4).Infof("Should start checkpoint %s", id)
-			start = append(start, id)
+	// The apiserver pod is deleted, transition to stateInactiveGracePeriod.


the apiserver parent pod?

aaronlevy · 2017-10-31T18:56:19Z

pkg/checkpoint/process.go

-	for k := range removeMap {
-		if k == podCheckpointerID {
-			continue
+	// The apiserver pod is still deleted, remain in stateInactiveGracePeriod.


apiserver parent pod?

diegs · 2017-10-31T20:27:35Z

coreosbot run e2e checkpointer

diegs · 2017-10-31T20:30:03Z

@aaronlevy thanks for the comments. Made a few small changes, also testing it against tectonic and it seems to be surviving apiserver destruction nicely

diegs · 2017-10-31T20:30:43Z

coreosbot run e2e checkpointer

yifan-gu · 2017-10-31T21:50:32Z

pkg/checkpoint/manifest.go

@@ -73,3 +69,11 @@ func writeManifestIfDifferent(path, name string, data []byte) (bool, error) {
 	glog.Infof("Writing manifest for %q to %q", name, path)
 	return true, writeAndAtomicRename(path, data, 0644)
 }
+
+func writeAndAtomicRename(path string, data []byte, perm os.FileMode) error {
+	tmpfile := filepath.Join(filepath.Dir(path), "."+filepath.Base(path))


Now I notice that maybe we should not write the file under the directory.
Even with the prefix of ".", it will still be scanned by ReadDir() in func getFileCheckpoints().

@yifan-gu since the checkpointer is single-threaded getFileCheckpoints() should not run concurrently. The only problem is if the program crashes in the middle of writeAndAtomicRename and we have a leftover .foo.json file sitting there.

I can adjust getFileCheckpoints() to ignore files that start with ., WDYT?

I guess my question is why we are not just using a system temp dir here?

@yifan-gu Atomic renames are only guaranteed in the same directory.

@yifan-gu added some code in getFileCheckpoints() to clean up old files that start with .

https://godoc.org/os#Rename calls rename() says 'OS-specific restrictions may apply when oldpath and newpath are in different directories. If there is an error, it will be of type *LinkError.'

If you look at http://man7.org/linux/man-pages/man2/rename.2.html, EXDEV occurs if the old and new locations are on different filesystems (I've seen this happen before). It's safer to keep it in the same directory.

ioutil.TempFile would be a smart choice to create a temporary file within a certain directory

@rphillips the only difference would be that it will choose a random rather than deterministic filename, as we would be calling it with the current directory and . prefix, and then writing to it... I can make the change anyway though.

it isn't a hard blocker... just a suggestion if we need to enumerate those files with a better prefix in the future...

Eh already changed it.

yifan-gu · 2017-10-31T22:34:17Z

pkg/checkpoint/process_test.go

-			t.Errorf("For test: %s\nExpected start: %s Got: %s\nExpected stop: %s Got: %s\nExpected remove: %s Got: %s\n",
-				tc.desc, tc.expectStart, gotStart, tc.expectStop, gotStop, tc.expectRemove, gotRemove)
+			!reflect.DeepEqual(tc.expectRemove, gotRemove) ||
+			!reflect.DeepEqual(tc.expectGraceStart, gotGraceStart) ||


nit: Maybe just print them one by one? I mean:

if !reflect.DeepEqual() { t.Errorf() } if !reflect.DeepEqual() { t.Errorf() } ...

If there are multiple states this can get a little verbose. I prefer to keep it as is.

yifan-gu · 2017-10-31T23:04:53Z

pkg/checkpoint/process.go

+	return starts, stops, removes
+}
+
+// stateSelfCheckpoint represents a checkpoint of the checkpointer itself, which has special


maybe define all these states in a separate file? this process.go is getting big, I think it would be cleaner to just keep control logics here.

yifan-gu · 2017-10-31T23:09:10Z

pkg/checkpoint/process.go

+}
+
+// state represents the current state of a checkpoint.
+type state interface {


Was a little confused by the name at first because we also have apiState, either change apiState to something else like apiCondition, or rename this to podState?

Done (both)

diegs · 2017-10-31T23:31:40Z

@yifan-gu thanks for the review! PTAL

diegs · 2017-10-31T23:31:55Z

coreosbot run e2e checkpointer

kubernetes-retired/bootkube#755

s-urbaniak · 2017-11-01T12:29:35Z

Thanks a lot @diegs, first time coreos/tectonic-installer#1938 passed green with this one 👍

kubernetes-retired/bootkube#755

yifan-gu · 2017-11-01T19:17:33Z

pkg/checkpoint/state.go

+// behavior.
+//
+// stateSelfCheckpoint can transition to stateInactiveGracePeriod.
+type stateSelfCheckpoint struct{}


nit: stateSelfCheckpointActive ?

yifan-gu

I like the direction, overall LGTM.
I hope to see some more unit tests that exercises all the transition path between those states in the future, not a blocker.

This implements an explicit state machine to capture the various states a checkpoint can be in. It recovers those states from disk upon startup (if applicable) and then reconciles them with the states that are fetched from the various apiservers. It also adds 2 new states to attempt to work around issues with kubernetes 1.8 in which daemonset pods are deleted before being recreated. For single-master clusters this can lead to a total outage during apiserver upgrades since the checkpointer will aggressively retire the checkpoint for the deleted apiserver pod before it has a chance to schedule the new one.

diegs · 2017-11-01T20:32:38Z

@yifan-gu thanks, I added a simple test to enforce only allowed transitions can occur. It doesn't actually check behavior (I was relying on the existing test for that) but I think it's a small improvement for now.

diegs · 2017-11-01T20:32:46Z

coreosbot run e2e checkpointer

yifan-gu

LGTM

If the checkpointer crashes during `writeAndAtomicRename()` then it may leave temporary files that start with `.`. This checks for and removes them. Also changes `writeAndAtomicRename()` to use `ioutil.TempFile()`.

diegs · 2017-11-01T20:56:29Z

coreosbot run e2e checkpointer

diegs · 2017-11-01T20:57:51Z

coreosbot run e2e checkpointer

diegs added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Oct 28, 2017

diegs self-assigned this Oct 28, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 28, 2017

diegs force-pushed the cp-cooldown branch from 1c1757d to 3787d54 Compare October 31, 2017 00:25

checkpointer: small cleanups from refactor.

d3f812d

diegs force-pushed the cp-cooldown branch from 3787d54 to b5c011a Compare October 31, 2017 04:27

diegs changed the title ~~WIP checkpointer: implement state machine.~~ checkpointer: implement state machine. Oct 31, 2017

diegs force-pushed the cp-cooldown branch from b5c011a to 42ec725 Compare October 31, 2017 04:37

diegs removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Oct 31, 2017

diegs requested a review from aaronlevy October 31, 2017 04:37

diegs requested a review from dghubble October 31, 2017 05:26

mxinden reviewed Oct 31, 2017

View reviewed changes

aaronlevy reviewed Oct 31, 2017

View reviewed changes

diegs force-pushed the cp-cooldown branch 3 times, most recently from b9d2b16 to 4544738 Compare October 31, 2017 20:11

diegs force-pushed the cp-cooldown branch from 4544738 to 13bd57e Compare October 31, 2017 20:30

yifan-gu reviewed Oct 31, 2017

View reviewed changes

diegs force-pushed the cp-cooldown branch from 13bd57e to 9323057 Compare October 31, 2017 23:31

s-urbaniak pushed a commit to s-urbaniak/tectonic-installer that referenced this pull request Nov 1, 2017

WIP: test pod-checkpointer DO-NOT-MERGE

711be10

kubernetes-retired/bootkube#755

s-urbaniak mentioned this pull request Nov 1, 2017

modules/*: bump to k8s v1.8.2 coreos/tectonic-installer#1938

Merged

s-urbaniak pushed a commit to s-urbaniak/tectonic-installer that referenced this pull request Nov 1, 2017

WIP: test pod-checkpointer and Docker 17 DO-NOT-MERGE

fbacd19

kubernetes-retired/bootkube#755

s-urbaniak pushed a commit to s-urbaniak/tectonic-installer that referenced this pull request Nov 1, 2017

WIP: test pod-checkpointer and Docker 17 DO-NOT-MERGE

80b45fa

kubernetes-retired/bootkube#755

yifan-gu reviewed Nov 1, 2017

View reviewed changes

yifan-gu approved these changes Nov 1, 2017

View reviewed changes

diegs force-pushed the cp-cooldown branch from 9323057 to 1d33f0f Compare November 1, 2017 19:27

diegs force-pushed the cp-cooldown branch from 1d33f0f to 730f2ad Compare November 1, 2017 20:31

yifan-gu approved these changes Nov 1, 2017

View reviewed changes

diegs force-pushed the cp-cooldown branch from 730f2ad to b50ee3e Compare November 1, 2017 20:53

checkpoint: clean up orphaned checkpoints.

02785c9

If the checkpointer crashes during `writeAndAtomicRename()` then it may leave temporary files that start with `.`. This checks for and removes them. Also changes `writeAndAtomicRename()` to use `ioutil.TempFile()`.

diegs force-pushed the cp-cooldown branch from b50ee3e to 02785c9 Compare November 1, 2017 20:56

rphillips approved these changes Nov 1, 2017

View reviewed changes

diegs merged commit 69f99ef into kubernetes-retired:master Nov 1, 2017

diegs deleted the cp-cooldown branch November 1, 2017 22:44

aaronlevy mentioned this pull request Dec 12, 2017

Simple scheduler & controller-manager disaster recovery #112

Closed

diegs mentioned this pull request Dec 20, 2017

checkpointer: make grace period a flag. #813

Merged

checkpointer: implement state machine. #755

checkpointer: implement state machine. #755

Conversation

diegs commented Oct 28, 2017 • edited Loading

diegs commented Oct 31, 2017

diegs commented Oct 31, 2017

diegs commented Oct 31, 2017

mxinden left a comment

Choose a reason for hiding this comment

diegs commented Oct 31, 2017

aaronlevy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegs commented Oct 31, 2017

diegs commented Oct 31, 2017

diegs commented Oct 31, 2017

yifan-gu Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegs commented Oct 31, 2017

diegs commented Oct 31, 2017

s-urbaniak commented Nov 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yifan-gu left a comment

Choose a reason for hiding this comment

diegs commented Nov 1, 2017

diegs commented Nov 1, 2017

yifan-gu left a comment

Choose a reason for hiding this comment

diegs commented Nov 1, 2017

diegs commented Nov 1, 2017

diegs commented Oct 28, 2017 •

edited

Loading

yifan-gu Oct 31, 2017 •

edited

Loading